Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct order of test based on #618 #619

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Conversation

mjp41
Copy link
Member

@mjp41 mjp41 commented Jun 20, 2023

No description provided.

@mjp41 mjp41 requested a review from nwf-msr June 20, 2023 13:45
@nwf-msr
Copy link
Contributor

nwf-msr commented Jun 20, 2023

I think this is right, so long as the allocator client is correct. To expand on that a bit, the concern is that invariant() could interleave some other sequence of (correct!) operations such that it observes front() being meaningful (that is, not the stub) and then, later, observes back being nullptr. The only things that can set back to nullptr are init() and destroy(), both of which only execute on the (singular) consumer thread and which should be gated by a global invariant: the consumer can only (re)initialize its message queue if all possible messages have already been received, in which case, there won't be any producer threads with which to interleave.

However, that neglects remote double-free. If a deallocating thread makes it as far as getting into

template<typename Domesticator_head>
void enqueue(
freelist::HeadPtr first,
freelist::HeadPtr last,
Domesticator_head domesticate_head)
{
invariant();

and is preempted for a long while, across a double free and frees of all the other objects for the same remote allocator and that remote allocator getting torn down, it could indeed see a meaningful front and then (much later) a nullptr back.

@nwf-msr
Copy link
Contributor

nwf-msr commented Jun 20, 2023

I think we will end up (at least in the Cornucopia prototypes and the eventual CHERI+MTE world) with double-free guards being rather towards the front, before we could even get to enqueue()ing to the remote queue. In the current stochastic detection regime, we're tripping an assert when we detect double-frees anyway, and we're much more likely to cause double-frees to trip over the intended "congratulations, you corrupted the message queue" assert(s) rather than this one.

@mjp41
Copy link
Member Author

mjp41 commented Jun 20, 2023

I think we will end up (at least in the Cornucopia prototypes and the eventual CHERI+MTE world) with double-free guards being rather towards the front, before we could even get to enqueue()ing to the remote queue. In the current stochastic detection regime, we're tripping an assert when we detect double-frees anyway, and we're much more likely to cause double-frees to trip over the intended "congratulations, you corrupted the message queue" assert(s) rather than this one.

So this is a SNMALLOC_ASSERT not a SNMALLOC_CHECK, so is more for debugging the allocator rather than detecting user errors. I think the double free protection would catch issues much better than this assert.

@mjp41 mjp41 merged commit 95bad42 into microsoft:main Jun 20, 2023
@mjp41 mjp41 deleted the correct_invariant branch June 20, 2023 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants